feat(bindings): add fromTemp metadata to localstorage#4280
feat(bindings): add fromTemp metadata to localstorage#4280
Conversation
Signed-off-by: stywzn <jhonynnis@gmail.com>
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
|
@dapr-bot remove-stale |
|
@stywzn It'd be good if you improve the certification tests to use this new setting too |
There was a problem hiding this comment.
Pull request overview
Adds a fromTemp metadata flag to the localstorage output binding so rootPath can be resolved under the OS temp directory (via os.TempDir()), improving cross-platform dev setups.
Changes:
- Document
fromTempinbindings/localstorage/metadata.yaml. - Extend
Metadataand updateparseMetadatato prefixrootPathwithos.TempDir()whenfromTemp=true. - Add a unit test validating the resolved temp-based
RootPath.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| bindings/localstorage/metadata.yaml | Documents the new fromTemp metadata option. |
| bindings/localstorage/localstorage.go | Adds FromTemp field and temp-dir resolution logic in metadata parsing. |
| bindings/localstorage/localstorage_test.go | Adds coverage for fromTemp=true happy-path parsing behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| if m.FromTemp { | ||
| m.RootPath = filepath.Join(os.TempDir(), m.RootPath) |
There was a problem hiding this comment.
When fromTemp is enabled, filepath.Join(os.TempDir(), m.RootPath) can be bypassed: an absolute rootPath will cause Join to ignore os.TempDir(), and values with .. can escape the temp directory. Also, an empty rootPath becomes the temp directory itself, bypassing the existing "rootPath must not be empty" validation. Consider rejecting empty/absolute paths when fromTemp is true and using securejoin.SecureJoin(os.TempDir(), m.RootPath) (or equivalent) to guarantee the resolved path stays under the temp dir before calling validateRootPath.
| m.RootPath = filepath.Join(os.TempDir(), m.RootPath) | |
| if strings.TrimSpace(m.RootPath) == "" { | |
| return nil, errors.New("rootPath must not be empty") | |
| } | |
| if filepath.IsAbs(m.RootPath) { | |
| return nil, errors.New("rootPath must not be absolute when fromTemp is enabled") | |
| } | |
| m.RootPath, err = securejoin.SecureJoin(os.TempDir(), m.RootPath) | |
| if err != nil { | |
| return nil, fmt.Errorf("invalid rootPath for temp directory: %w", err) | |
| } |
| RootPath string `json:"rootPath"` | ||
| FromTemp bool `json:"fromTemp"` |
There was a problem hiding this comment.
GetComponentMetadata relies on metadata.GetMetadataInfoFromStructType, which uses mapstructure tags (and otherwise falls back to exported field names). Since these fields only have json tags, the metadata map will expose keys like RootPath/FromTemp rather than rootPath/fromTemp, diverging from metadata.yaml and other components. Consider adding mapstructure:"rootPath" and mapstructure:"fromTemp" tags (keeping the existing json tags if needed) so metadata introspection and decoding are consistent.
| RootPath string `json:"rootPath"` | |
| FromTemp bool `json:"fromTemp"` | |
| RootPath string `json:"rootPath" mapstructure:"rootPath"` | |
| FromTemp bool `json:"fromTemp" mapstructure:"fromTemp"` |
| description: "The file name to write" | ||
| example: "data.txt" | ||
| - name: fromTemp | ||
| description: "If true, resolves rootPath from the system-wide temporary directory." |
There was a problem hiding this comment.
The description says "system-wide temporary directory", but os.TempDir() returns the process default temp directory (often influenced by env vars and frequently user-specific on Windows). Consider rewording to "system temporary directory"/"default temporary directory (os.TempDir())" to avoid overpromising behavior.
| description: "If true, resolves rootPath from the system-wide temporary directory." | |
| description: "If true, resolves rootPath from the default temporary directory (os.TempDir())." |
| m.Properties = map[string]string{ | ||
| "rootPath": "myapp_data", | ||
| "fromTemp": "true", | ||
| } | ||
| metaTemp, err := localStorage.parseMetadata(m) | ||
| require.NoError(t, err) | ||
|
|
||
| realTempDir, err := filepath.EvalSymlinks(os.TempDir()) | ||
| require.NoError(t, err) | ||
| expectedTempPath := filepath.Join(realTempDir, "myapp_data") | ||
|
|
||
| assert.Equal(t, expectedTempPath, metaTemp.RootPath) | ||
| assert.True(t, metaTemp.FromTemp) | ||
| } |
There was a problem hiding this comment.
TestParseMetadata covers the happy path for fromTemp=true, but doesn't exercise edge cases introduced by the new flag (for example: missing/empty rootPath, absolute rootPath, or rootPath containing ..). Adding assertions for these cases would help prevent regressions and verify that fromTemp cannot resolve outside the temp directory.
Description
This PR adds the
fromTempboolean metadata to thelocalstoragebinding component to allow resolving therootPathfrom the system-wide temporary directory usingos.TempDir(). This ensures true cross-platform compatibility.This addresses the need for using system temp directories in development scenarios without hardcoding OS-specific paths (like
/tmporC:\Windows\Temp). Downward compatibility is strictly maintained (defaults tofalse). Unit tests andmetadata.yamlhave been updated accordingly.Fixes #3158